Block Bindings: do not use useSource hook conditionally #59403

retrofox · 2024-02-27T13:24:16Z

What?

This pull request enhances the mechanism by which block attributes are bound to external source properties. The primary aim is to prevent calling React hooks in the wrong way, in this case, conditionally.

On this PR:

Component Introduction: Two new components, <BlockBindingBridge /> and <BlockBindingConnector />, have been introduced. These components isolate and encapsulate connecting block attributes with external source properties.
Hook Invocation Improvement: To circumvent the issue of incorrect hook invocation within a loop, the source object is now passed down to the <BlockBindingConnector />. This ensures that the hook is called at the top level of the component, aligning with React's best practices.
API Modification: The useSource() API has changed. Previously, a useValue method was provided, but it has been removed in favor of directly exposing the source property's value and its handler through value and updateValue props, respectively. The updated API structure is as follows:

export default {
  name: 'namespace/source-handler',
  label: 'The custom namespace source-handler',
  useSource: (blockProps, sourceArgs) => {
    // Logic to fetch value
    const value = getFromWherever();
    const updateValue = () => {};
    // Expose value and its updater
    return {
      placeholder,
      value,
      updateValue,
    };
  }
};

Why?

Because calling the hook wrongly is not recommended. It could bring performance issues.

How?

This PR addresses the issue by introducing and components to streamline the connection between block attributes and external sources, thereby preventing incorrect hook calls. It refines the useSource() API to directly expose the source property's value.

Testing Instructions

Pretty similar to #58085

Register a new custom field however you prefer. You can use a snippet similar to this:

register_meta(
	'post',
	'text_custom_field',
	array(
		'show_in_rest' => true,
		'single'       => true,
		'type'         => 'string',
		'default'	   => 'This is the content of the text custom field',
	)
);
register_meta(
	'post',
	'url_custom_field',
	array(
		'show_in_rest' => true,
		'single'       => true,
		'type'         => 'string',
		'default'	   => 'https://wpmovies.dev/wp-content/uploads/2023/04/goncharov-poster-original-1-682x1024.jpeg',
	)
);

Test paragraph

Add a paragraph with the content connected to a custom field:

<!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"core/post-meta","args":{"key":"text_custom_field"}}}}} -->
<p>Value provided by the source (core/post-meta)</p>
<!-- /wp:paragraph -->

Confirm it isn't possible to edit the block content.
Confirm that if the meta text_custom_field value is defined ((001) Meta value in my testing block), then it should be shown in the UI:

packages/block-editor/src/hooks/use-bindings-attributes.js

gziolo

Code-wise it is looking good enough to include in WP 6.5 RC1. I will do some testing to ensure it all works as expected.

SantosGuillamot · 2024-02-28T10:23:28Z

Not 100% related to this pull request, but it will affect once this other one gets merged.

There is an issue with the fallback value that is solved by this refactoring. However, we will need to update the e2e tests after the mentioned PR is merged. We'll have to update these lines to have "fallback value": line 1, line 2, & line 3.

gziolo

Everything tests well for me, and I also see that all e2e tests on CI pass which means we should be good to go.

There is still a tiny issue that is also present in trunk that I hoped this PR would also resolve. However, I prefer we explore it separately. I included a screencast with CPU throttling enabled at 6x, but it might still need watching the video at slower pace:

Screen.Recording.2024-02-28.at.12.51.50.mov

That might be specific to the Query Loop block and its implementation. We can observe that when changing the active post, the whole Post Template renders from scratch, and in that case, you can see the fallback value stored in HTML for blocks, which gets replaced in the next render with the value coming from the source.

The great news is that the fallback loaded from the saved content is never replaced with the value from the source, which is the intended behavior I advocated to have in the WP 6.5 release.

SantosGuillamot · 2024-02-28T13:24:34Z

As explained in this comment, I've updated the tests in this commit now that the mentioned PR has been merged.

SantosGuillamot · 2024-02-28T13:36:01Z

There is still a tiny issue that is also present in trunk that I hoped this PR would also resolve. However, I prefer we explore it separately.

I have just pushed this commit that should solve the blinking effect. At least until we figure out a better solution.

* replace use-binding-attributes with block-binding-support * minor enhancement * minor change * tweak * do not import use-binding-attributes * use isItPossibleToBindBlock() helper * introduce core/entity source handler * rename folder * rename source name * polish post-entity source handler * make core/post-entity more consistent with core-data * make entity source hand;ler more generic * fix entity sour handl;er issues * remove uneeded useValue () hook (crossfingers) * minor jsdoc improvement * clean * rename with updateValue() * remove core/entity binding source handler * move useSource to Connector cmp * move the whole dryining logic to the Connect component * improve jsdoc * rename to blockProps * minor jsdoc improvements * use a single effect to update attr and value * discard useValue. Return value and setValue instead * check wheter updateValue function is defined * check prop value is defined when updating attr * handle `placerholder` * ensure to put attribute in sync when onmount * remove // eslint comment * enable editing for bound with post-meta * move block bindiung processor to hooks/ * ensure update bound attr once when mounting * Update packages/block-editor/src/hooks/block-binding-support/index.js Co-authored-by: Michal <[email protected]> * disable editing block attribute * move changes to the use-binding-attributes file * introduce BlockBindingBridge component * update isItPossibleToBindBlock() import path * introduce hasPossibleBlockBinding() helper * use hooks API to extened blocks with bound attts * fix propagating attr value. jsdoc * minor changes * minor code enhancement * not edit bound prop for now * jsdoc * revert using hooks API to extrend block * jsdoc * update internal path * rollback hook utils chnages * tidy * wrap Connector instances with a Fragment * return original Edit instance when no bindings * check whether useSource is defined * Use `useSelect` and move it out of the for loop * check attr value type * iterare when creating BindingConnector instances * rename helper functions * use useSelect to get binding sources * Update packages/block-editor/src/hooks/use-bindings-attributes.js Co-authored-by: Michal <[email protected]> * Update packages/block-editor/src/hooks/use-bindings-attributes.js Co-authored-by: Michal <[email protected]> * pass prev attr value to compare * improve binding allowed block attributes * sync derevied updates when updating bound attr * improve getting attr source * check properly bindings data * preserve the RichTextData for block attr * comment line just for tesrting purposes * rebasing changes * rollback change foir testing purposes * change cmp prop name. improve jsdoc * simplify checking bindins value * use attr name as key instance * store bound attrs values in a local state * collect and update bound attr in a local state * Refactor block binding functionality from e55f6bc * pick block data from straight props * remove conditional onPropValueChange call * Update e2e tests * Use `useLayoutEffect` instead of `useEffect` --------- Co-authored-by: Michal <[email protected]> Co-authored-by: Mario Santos <[email protected]>

youknowriad · 2024-03-04T09:07:45Z

I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: e88f867

* replace use-binding-attributes with block-binding-support * minor enhancement * minor change * tweak * do not import use-binding-attributes * use isItPossibleToBindBlock() helper * introduce core/entity source handler * rename folder * rename source name * polish post-entity source handler * make core/post-entity more consistent with core-data * make entity source hand;ler more generic * fix entity sour handl;er issues * remove uneeded useValue () hook (crossfingers) * minor jsdoc improvement * clean * rename with updateValue() * remove core/entity binding source handler * move useSource to Connector cmp * move the whole dryining logic to the Connect component * improve jsdoc * rename to blockProps * minor jsdoc improvements * use a single effect to update attr and value * discard useValue. Return value and setValue instead * check wheter updateValue function is defined * check prop value is defined when updating attr * handle `placerholder` * ensure to put attribute in sync when onmount * remove // eslint comment * enable editing for bound with post-meta * move block bindiung processor to hooks/ * ensure update bound attr once when mounting * Update packages/block-editor/src/hooks/block-binding-support/index.js Co-authored-by: Michal <[email protected]> * disable editing block attribute * move changes to the use-binding-attributes file * introduce BlockBindingBridge component * update isItPossibleToBindBlock() import path * introduce hasPossibleBlockBinding() helper * use hooks API to extened blocks with bound attts * fix propagating attr value. jsdoc * minor changes * minor code enhancement * not edit bound prop for now * jsdoc * revert using hooks API to extrend block * jsdoc * update internal path * rollback hook utils chnages * tidy * wrap Connector instances with a Fragment * return original Edit instance when no bindings * check whether useSource is defined * Use `useSelect` and move it out of the for loop * check attr value type * iterare when creating BindingConnector instances * rename helper functions * use useSelect to get binding sources * Update packages/block-editor/src/hooks/use-bindings-attributes.js Co-authored-by: Michal <[email protected]> * Update packages/block-editor/src/hooks/use-bindings-attributes.js Co-authored-by: Michal <[email protected]> * pass prev attr value to compare * improve binding allowed block attributes * sync derevied updates when updating bound attr * improve getting attr source * check properly bindings data * preserve the RichTextData for block attr * comment line just for tesrting purposes * rebasing changes * rollback change foir testing purposes * change cmp prop name. improve jsdoc * simplify checking bindins value * use attr name as key instance * store bound attrs values in a local state * collect and update bound attr in a local state * Refactor block binding functionality from e55f6bc * pick block data from straight props * remove conditional onPropValueChange call * Update e2e tests * Use `useLayoutEffect` instead of `useEffect` --------- Co-authored-by: Michal <[email protected]> Co-authored-by: Mario Santos <[email protected]>

retrofox added 30 commits February 26, 2024 11:36

replace use-binding-attributes with block-binding-support

37a7f5a

minor enhancement

09f8565

minor change

45742ca

tweak

6af2517

do not import use-binding-attributes

f02c4ef

use isItPossibleToBindBlock() helper

6412b7d

introduce core/entity source handler

5749a34

rename folder

25b0bb8

rename source name

76e0cb5

polish post-entity source handler

dc48e09

make core/post-entity more consistent with core-data

44421f1

make entity source hand;ler more generic

8a11812

fix entity sour handl;er issues

1b40bef

remove uneeded useValue () hook (crossfingers)

59b26ba

minor jsdoc improvement

72338e3

clean

f7d87a8

rename with updateValue()

b60c7ea

remove core/entity binding source handler

adfd711

move useSource to Connector cmp

832f170

move the whole dryining logic to the Connect component

42fc47c

improve jsdoc

09c8cd3

rename to blockProps

b0a0310

minor jsdoc improvements

c46c140

use a single effect to update attr and value

4f4e3b5

discard useValue. Return value and setValue instead

5ae157e

check wheter updateValue function is defined

6ebb240

check prop value is defined when updating attr

8c8c90c

handle placerholder

536bc90

ensure to put attribute in sync when onmount

4063d77

remove // eslint comment

6a28f73

gziolo added the [Package] Block editor /packages/block-editor label Feb 28, 2024

gziolo reviewed Feb 28, 2024

View reviewed changes

packages/block-editor/src/hooks/use-bindings-attributes.js Outdated Show resolved Hide resolved

gziolo reviewed Feb 28, 2024

View reviewed changes

gziolo mentioned this pull request Feb 28, 2024

Block Bindings: Don't show protected fields that are bound to blocks #59326

Merged

retrofox added 2 commits February 28, 2024 08:53

remove conditional onPropValueChange call

fa9ed05

Merge branch 'trunk' into update/do-not-conditional-hook-in-binding

021f87a

gziolo approved these changes Feb 28, 2024

View reviewed changes

Update e2e tests

b6d14b5

retrofox requested review from kevin940726 and Mamaduka as code owners February 28, 2024 13:23

Use useLayoutEffect instead of useEffect

879129c

michalczaplinski mentioned this pull request Feb 28, 2024

Block bindings: Don't use useEffect in the block bindings editor hook #59443

Closed

retrofox merged commit faf60a4 into trunk Feb 28, 2024
59 of 60 checks passed

retrofox deleted the update/do-not-conditional-hook-in-binding branch February 28, 2024 15:56

github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024

youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 4, 2024

youknowriad mentioned this pull request Mar 4, 2024

Cherry pick commits for WP 6.5 RC 1 #59541

Merged

retrofox mentioned this pull request Mar 4, 2024

(POC) Block Bindings: edit external prop value/block attribute #59537

Closed

SantosGuillamot mentioned this pull request Mar 13, 2024

Connecting block attributes and custom fields & Block Bindings API for WP 6.5 #53300

Closed

42 tasks

retrofox mentioned this pull request Apr 1, 2024

Block Bindings: set allow blocks list from source handler #60335

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Bindings: do not use useSource hook conditionally #59403

Block Bindings: do not use useSource hook conditionally #59403

retrofox commented Feb 27, 2024 •

edited

Loading

gziolo left a comment

SantosGuillamot commented Feb 28, 2024

gziolo left a comment

SantosGuillamot commented Feb 28, 2024

SantosGuillamot commented Feb 28, 2024

youknowriad commented Mar 4, 2024

Block Bindings: do not use useSource hook conditionally #59403

Block Bindings: do not use useSource hook conditionally #59403

Conversation

retrofox commented Feb 27, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

gziolo left a comment

Choose a reason for hiding this comment

SantosGuillamot commented Feb 28, 2024

gziolo left a comment

Choose a reason for hiding this comment

SantosGuillamot commented Feb 28, 2024

SantosGuillamot commented Feb 28, 2024

youknowriad commented Mar 4, 2024

retrofox commented Feb 27, 2024 •

edited

Loading